Conversation
…trema-collation-20260924
github-actions
Bot
added
enhancement
sunchao
added
run-all-spark-profiles
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: DataFusion extrema ordering could select different floating-point or nested winners than Spark and did not honor nonbinary collations.
- Design approach: Share one
SparkArrayExtremaimplementation, with typed float scans, recursive comparisons, and per-string-leaf collation metadata. - Correctness / compatibility analysis: Checked Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and the 4.2 branch. First-winner retention, NaNs, signed zeros, nested null ordering, and supported UTF8 collations match the inspected Spark semantics. Unsupported collations retain dispatch/fallback and explicit incompatible opt-in.
- Key design decisions: Pin Unicode 16 mappings, supplement Unicode 17, preserve original winning values, and keep existing cast/scan boundaries. Comparators are constructed per batch without allocating lowercase strings per comparison.
- Implementation sketch: Scala serializes collations into
ScalarFunc, the planner constructs the configured UDF, and native evaluation copies selected results with bounded retained nested buffers. The abstraction stays local to extrema. - Behavioral changes worth calling out: The full stack fixes floating-point and nested ordering in both strict modes and enables native UTF8 collation comparisons. Reported performance remains workload-dependent, including slower cases. I did not independently rerun benchmarks.
- Suggested improvements: None at P1/P2 priority. No introduced P1/P2 issues found within this review.
Reviewed the entire diff from de2e70291460ed0310428493d8c3aa5223293d08 to d44b19ae53f1048f9d19529b159ff0c227f85c00, including prerequisite commits. Confirmed non-draft status and read the supplied discussion snapshot plus prerequisite discussions. Earlier substantiated concerns are addressed.
Routed skills: review-comet-pr, review-comet-expression-pr, review-comet-ffi-pr (test-only JNI change), and review-comet-memory-pr (nested result allocation).
Exact-head CI: 47 successful checks, 14 skipped, none failed or pending. This includes native tests, 20 Comet suite jobs across Spark 3.4–4.2, and all seven Spark 4.1 SQL shards. Inspected logs confirm the new regressions ran. These checks attach to the requested head, while the workflow tested PR merge ref c1c0282. macOS and other Spark SQL versions were skipped.
Local validation: All 11 focused native extrema tests passed at the pinned head. An isolated check of the exact comparator passed exhaustive Unicode 16/17 mapping validation against ICU data and Spark’s special rules, plus 800,000 string comparisons. git diff --check passed. Local JVM/JNI integration and release benchmarks were not rerun.
1 participant
Which issue does this PR close?
Part of #4496. Depends on #5403, currently at
ae755c459e0acd95f72a53f538d959e2adec572c; rebase after it lands. The incremental follow-up diff starts after merging #5403 with this PR's main base.Rationale for this change
#5403 uses Spark's JVM dispatcher for collated
array_min/array_max. This follow-up evaluates the UTF8 collation family in Rust, including strings nested in arrays and structs. For example,array_minof['a', 'B']underUTF8_LCASEreturns the original'a'using the native extrema comparator.What changes are included in this PR?
UTF8_BINARY,UTF8_BINARY_RTRIM,UTF8_LCASE, andUTF8_LCASE_RTRIM, preserving the original first winner on ties.unicode-case-mapping1.0.0 for Unicode 16, with Spark's sigma rule and Unicode 17 additions. Unsupported collations or Unicode versions retain the existing dispatch/fallback and incompatible opt-in behavior.How are these changes tested?
At
895838be7bbb2eaf24da0ba28cf43ca4268e5b36:array_extrema_collation.sqlfixture, and the collation-cast fallback regression.git diff --checkpassed.426971841. All 24 release JNI Spark-versus-Comet cases completed, asserting native extrema while allowing existing input-cast dispatch.Hosted CI is running for this revision. The
run-all-spark-profilesandrun-spark-4.1-testslabels request the broader suites.Release benchmark results (Spark 4.1.3, JDK 17, AMD EPYC Milan, local single worker, 262,144 rows):
The table shows Spark/Comet mean-time ratios, ranging across
array_min/array_max; above 1 favors Comet. It includes input-cast dispatch and query execution. The binary control retains the existing scalar/DataFusion path.Results are workload-dependent, with substantial variance in some long-ASCII Comet cases. This is one local run of
CometArrayExpressionBenchmark.arrayExtremaCollationBenchmark, not a general speedup claim or a before/after measurement of the cast-fallback fix. The native library was built from this commit with the release profile; debug symbols were disabled.